Skip to content

C++: Detect non-allocating placement new in cpp/memory-never-freed#523

Merged
geoffw0 merged 2 commits into
github:masterfrom
jbj:placement-new-never-freed
Nov 23, 2018
Merged

C++: Detect non-allocating placement new in cpp/memory-never-freed#523
geoffw0 merged 2 commits into
github:masterfrom
jbj:placement-new-never-freed

Conversation

@jbj

@jbj jbj commented Nov 22, 2018

Copy link
Copy Markdown
Contributor

This addresses CPP-294. It's a more conservative solution than what we've talked about because I think a more complete solution is at risk of introducing false negatives. Therefore I'd prefer that proposals for a more complete solution should be based on actual FPs in actual projects.

jbj added 2 commits November 22, 2018 10:48
This adds a `NewOrNewArrayExpr.getPlacementPointer` predicate and uses
it in `Alloc.qll` to detect when a `new`-expression is not an
allocation.

User-defined replacements for `operator new` may not be allocations
either, but the code continues to assume that they are. It's possible
that we want to change this assumption in the future or leave it up to
individual queries to decide on which side to err. It's hard to
statically tell whether `operator new` has been overloaded in a
particular file because it can be overloaded by a definition that is not
in scope but is only linked together with that file.
@jbj jbj added the C++ label Nov 22, 2018
@jbj jbj added this to the 1.19 milestone Nov 22, 2018
@jbj jbj requested a review from a team as a code owner November 22, 2018 10:47
@jbj

jbj commented Nov 22, 2018

Copy link
Copy Markdown
Contributor Author

The test failures will be fixed by #525.

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jbj

jbj commented Nov 23, 2018

Copy link
Copy Markdown
Contributor Author

I think this is good to merge. I haven't re-run the Azure tests, but I've re-run Language-Tests/CPP after #525 was merged, and it passes now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants